Skip to content

Revised AV1553#193

Merged
dennisdoomen merged 1 commit into
dennisdoomen:masterfrom
keremispirli:patch-1
Sep 22, 2019
Merged

Revised AV1553#193
dennisdoomen merged 1 commit into
dennisdoomen:masterfrom
keremispirli:patch-1

Conversation

@keremispirli
Copy link
Copy Markdown
Contributor

The current guideline looks like a blanket ban which also affects legitimate uses of optional parameters that current justifications mentioned in the guideline do not apply.

If the optional parameter is a reference type then it can only have a default value of null. But since strings, lists and collections should never be null according to rule AV1135, you must use overloaded methods instead.

The claim in the first sentence is already wrong about String's: Even though “String” is reference type, an optional string parameter can have non-null default values. Also, the first sentence of the justification doesn't support the suggestion in the second sentence.

Note: The default values of the optional parameters are stored at the caller side. As such, changing the default value without recompiling the calling code will not apply the new default value.

Note: When an interface method defines an optional parameter, its default value is discarded during overload resolution unless you call the concrete class through the interface reference.

These two justifications written as notes do not apply to private or internal methods, neither does Eric Lippert's series of posts linked at the end.

Proposed changes:

  1. Removed the first sentence of the first justification. Not only that it's wrong with respect to string type, but also it doesn't add value even if we fix it.
  2. Added explanation for using optional string parameters.
  3. Extended first note to exclude private and internal methods, since it doesn't apply to them.
  4. Changed first note to a caveat, added a second caveat against abusing optional parameters to merge different methods.
  5. Converted second note to a separate guideline against using optional parameters in interfaces and their concrete implementations.

The current guideline looks like a blanket ban which also affects legitimate uses of optional parameters that current justifications mentioned in the guideline do not apply.

> If the optional parameter is a reference type then it can only have a default value of null. But since strings, lists and collections should never be null according to rule AV1135, you must use overloaded methods instead.

The claim in the first sentence is already wrong about String's: Even though “String” is reference type, an optional string parameter can have non-null default values. Also, the first sentence of the justification doesn't support the suggestion in the second sentence.

> Note: The default values of the optional parameters are stored at the caller side. As such, changing the default value without recompiling the calling code will not apply the new default value.
>
> Note: When an interface method defines an optional parameter, its default value is discarded during overload resolution unless you call the concrete class through the interface reference.

These two justifications written as notes do not apply to private or internal methods, neither does Eric Lippert's series of posts linked at the end.

Proposed changes:

1. Removed the first sentence of the first justification. Not only that it's wrong with respect to string type, but also it doesn't add value even if we fix it.
2. Added explanation for using optional string parameters.
3. Extended first note to exclude private and internal methods, since it doesn't apply to them.
4. Changed first note to a caveat, added a second caveat against abusing optional parameters to merge different methods.
5. Converted second note to a separate guideline against using optional parameters in interfaces and their concrete implementations.
@dennisdoomen
Copy link
Copy Markdown
Owner

Great suggestions indeed. Thanks for taken the time.

@dennisdoomen dennisdoomen merged commit e9273f9 into dennisdoomen:master Sep 22, 2019
@keremispirli keremispirli deleted the patch-1 branch September 22, 2019 08:34
@keremispirli
Copy link
Copy Markdown
Contributor Author

My pleasure. Thank you for creating this in the first place.

keremispirli added a commit to keremispirli/CSharpGuidelines that referenced this pull request Sep 22, 2019
(Piggyback) Removes _Empty lines_ entry regarding `#region` keyword, aligning with dennisdoomen#192
dennisdoomen pushed a commit that referenced this pull request Sep 22, 2019
(Piggyback) Removes _Empty lines_ entry regarding `#region` keyword, aligning with #192
mapfel pushed a commit to stepahead/CSharpGuidelines that referenced this pull request Mar 22, 2021
The current guideline looks like a blanket ban which also affects legitimate uses of optional parameters that current justifications mentioned in the guideline do not apply.

> If the optional parameter is a reference type then it can only have a default value of null. But since strings, lists and collections should never be null according to rule AV1135, you must use overloaded methods instead.

The claim in the first sentence is already wrong about String's: Even though “String” is reference type, an optional string parameter can have non-null default values. Also, the first sentence of the justification doesn't support the suggestion in the second sentence.

> Note: The default values of the optional parameters are stored at the caller side. As such, changing the default value without recompiling the calling code will not apply the new default value.
>
> Note: When an interface method defines an optional parameter, its default value is discarded during overload resolution unless you call the concrete class through the interface reference.

These two justifications written as notes do not apply to private or internal methods, neither does Eric Lippert's series of posts linked at the end.

Proposed changes:

1. Removed the first sentence of the first justification. Not only that it's wrong with respect to string type, but also it doesn't add value even if we fix it.
2. Added explanation for using optional string parameters.
3. Extended first note to exclude private and internal methods, since it doesn't apply to them.
4. Changed first note to a caveat, added a second caveat against abusing optional parameters to merge different methods.
5. Converted second note to a separate guideline against using optional parameters in interfaces and their concrete implementations.
mapfel pushed a commit to stepahead/CSharpGuidelines that referenced this pull request Mar 22, 2021
(Piggyback) Removes _Empty lines_ entry regarding `#region` keyword, aligning with dennisdoomen#192
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants